Skip to content

Add unit test for scalar vs. CGSize1 probing#509

Merged
sleeepyjack merged 4 commits intoNVIDIA:devfrom
sleeepyjack:test-scalar-probing
Jun 20, 2024
Merged

Add unit test for scalar vs. CGSize1 probing#509
sleeepyjack merged 4 commits intoNVIDIA:devfrom
sleeepyjack:test-scalar-probing

Conversation

@sleeepyjack
Copy link
Collaborator

@sleeepyjack sleeepyjack commented Jun 19, 2024

This PR adds a unit test to verify that the probing sequences for the scalar probing iterator vs. the iterator that uses a cooperative group of size 1 are actually equivalent.

@sleeepyjack sleeepyjack self-assigned this Jun 19, 2024
@sleeepyjack sleeepyjack requested a review from PointKernel as a code owner June 19, 2024 00:13
@sleeepyjack
Copy link
Collaborator Author

sleeepyjack commented Jun 19, 2024

Found the culprit with double hashing:
Apparently this expression in the scalar code path is different from this one in the CG code path. I'd have to wrap my head around that logic again to fix this. Alternatively we could internally dispatch to the CG code path in the CG probing iterator when CGSize==1 and call it a day.

@PointKernel
Copy link
Member

PointKernel commented Jun 19, 2024

With #501 fixing the sanitize hash logic, removing max from the scalar probing should fix the issue.

@sleeepyjack sleeepyjack mentioned this pull request Jun 19, 2024
Merged
@sleeepyjack sleeepyjack added the Needs Review Awaiting reviews before merging label Jun 19, 2024
@sleeepyjack
Copy link
Collaborator Author

sleeepyjack commented Jun 19, 2024

that fix broke something else: https://github.com/NVIDIA/cuCollections/actions/runs/9584556889/job/26440581801?pr=509#step:7:758

@PointKernel
Copy link
Member

PointKernel commented Jun 19, 2024

that fix broke something else: https://github.com/NVIDIA/cuCollections/actions/runs/9584556889/job/26440581801?pr=509#step:7:758

Which fix are you referring to? removing max from double hashing?

@sleeepyjack
Copy link
Collaborator Author

sleeepyjack commented Jun 19, 2024

Which fix are you referring to? removing max from double hashing?

Operator presedence: f0ae693

Not sure where my logic is flawed. 😢

@PointKernel
Copy link
Member

PointKernel commented Jun 20, 2024

Which fix are you referring to? removing max from double hashing?

Operator presedence: f0ae693

Not sure where my logic is flawed. 😢

No good way but printing out all probing steps. I will take a look tomorrow.

PointKernel
PointKernel approved these changes Jun 20, 2024
View reviewed changes
Copy link
Member

@PointKernel PointKernel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be ready to go.

@sleeepyjack
Copy link
Collaborator Author

sleeepyjack commented Jun 20, 2024

You sir are the MVP

@sleeepyjack sleeepyjack merged commit 6c0d7ee into NVIDIA:dev Jun 20, 2024
@PointKernel PointKernel added the type: bug Something isn't working label Jun 20, 2024
@sleeepyjack sleeepyjack deleted the test-scalar-probing branch June 20, 2024 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PointKernel PointKernel PointKernel approved these changes

Assignees

@sleeepyjack sleeepyjack

Labels

Needs Review Awaiting reviews before merging type: bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sleeepyjack @PointKernel